[PFA 3/n] Support PFA in const exprs - #22785
Conversation
arnaud-lb
changed the title
0ff8119 to
f5a4f79
Compare
arnaud-lb
changed the title
TimWolla
left a comment
TimWolla
left a comment
There was a problem hiding this comment.
I reviewed the PR excluding the first two commits. The tests LGTM. For the implementation, I didn't see obvious issues (except formatting nits), but I'm not fully comfortable approving the C changes.
0a6d9cd to
f153d7a
Compare
|
This can / should be rebased now. |
f153d7a to
9391556
Compare
|
I was doing it :) |
TimWolla
left a comment
TimWolla
left a comment
There was a problem hiding this comment.
Tests still LGTM. No longer seeing issues in the C implementation, but this is not my area of expertise.
9391556 to
b0ddbb6
Compare
iliaal
left a comment
iliaal
left a comment
There was a problem hiding this comment.
A few things from a read of the const-expr path (both correctness items are in paths no test covers, so CI stayed green).
… of the declaring opline itself zend_partial_create() takes a 'zend_op* declaring_opline' parameter for the purpose of building a cache key: the opline address is unique to this PFA as long as it's in SHM. However the declaring opline is not available when evaluating a PFA AST. Replace the parameter by 'uint32_t* declaring_lineno'. Callers can pass '&zend_op->lineno' or '&ast->lineno': Both are unique to the related PFA.
The declaring op_array is used for multiple purposes: * Generating a PFA name * Finding the filename * Finding whether op_array is cached, and therefore whether the PFA should be cached too However it's not available when evaluating a PFA AST. Remove the declaring_op_array argument, and pass the relevant information directly: * Generate the PFA name at compile time, pass it as argument to zend_partial_create() * Pass the filename and a cacheable flag as argument to zend_partial_create()
94287d3 to
a9a3b7b
Compare
iliaal
left a comment
iliaal
left a comment
There was a problem hiding this comment.
Both fixes look right. One note on the new regression case.
| } | ||
|
|
||
| try { | ||
| j(); |
There was a problem hiding this comment.
As committed this case still passes without the fix. With the cleanup fix reverted, running the file as-is exits 0; calling j() before f() and h(), or moving it to its own file, gives zend_mm_heap corrupted and exit 139.
Valgrind does flag the committed order (invalid read in zend_closure_free_storage, block freed at zend_ast.c:739), so an ASAN lane would catch it, but no ordinary lane will. Splitting j() into its own .phpt keeps it failing on unpatched code.
There was a problem hiding this comment.
Issues that are caught only by ASAN are fine, the entire test suite runs through ASAN and any failures there are considered to be blockers.
* Use a pointer to the declaring opline->lineno as unique addr, instead of the declaring opline itself zend_partial_create() takes a 'zend_op* declaring_opline' parameter for the purpose of building a cache key: the opline address is unique to this PFA as long as it's in SHM. However the declaring opline is not available when evaluating a PFA AST. Replace the parameter by 'uint32_t* declaring_lineno'. Callers can pass '&zend_op->lineno' or '&ast->lineno': Both are unique to the related PFA. * zend_partial_create(): Remove the dependency on the declaring op_array The declaring op_array is used for multiple purposes: * Generating a PFA name * Finding the filename * Finding whether op_array is cached, and therefore whether the PFA should be cached too However it's not available when evaluating a PFA AST. Remove the declaring_op_array argument, and pass the relevant information directly: * Generate the PFA name at compile time, pass it as argument to zend_partial_create() * Pass the filename and a cacheable flag as argument to zend_partial_create() * Support for PFA in constant expressions
3 participants
See individual commits. First two are GH-22783 and GH-22784, which will be merged separately. (Edit: these were merged, and the branch has been rebased.)